Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clean-up System.Drawing.Common and remove the Unix code. #64623

Merged
merged 16 commits into from
Apr 20, 2022
Merged

Clean-up System.Drawing.Common and remove the Unix code. #64623

merged 16 commits into from
Apr 20, 2022

Conversation

teo-tsirpanis
Copy link
Contributor

@teo-tsirpanis teo-tsirpanis commented Feb 1, 2022 •
edited
Loading

Since #64084 got merged, System.Drawing.Common is supported only on Windows. This PR actually removes all Unix-specific code and tests, and merges the .Windows.cs files into their formerly cross-platform counterparts, drastically simplifying the codebase.

I also cleaned-up the code a little bit (used a [ThreadStatic] instead of a named data slot, and removed unused files).

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 1, 2022
@ghost
Copy link

ghost commented Feb 1, 2022

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

Since #64084 got merged, System.Drawing.Common is supported only on Windows. This PR actually removes all Unix-specific code and tests, and merges the .Windows.cs files into their formerly cross-platform counterparts, drastically simplifying the codebase.

Author: teo-tsirpanis
Assignees: -
Labels:

area-System.Drawing, community-contribution
Milestone: -

@teo-tsirpanis teo-tsirpanis mentioned this pull request Feb 1, 2022
Closed
@safern
Copy link
Member

safern commented Feb 1, 2022

Thanks, @teo-tsirpanis -- please let me know when this is ready for review (CI is Red). I'd like to ask if you need any help to get CI green or a review, or just wait?

teo-tsirpanis
teo-tsirpanis commented Feb 1, 2022
View reviewed changes
Copy link
Contributor Author

@teo-tsirpanis teo-tsirpanis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for offering to help @safern. CI failures are so far unrelated (they occur on non-Windows platforms). I have two questions.

src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated Show resolved Hide resolved
src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj Outdated Show resolved Hide resolved
@ViktorHofer ViktorHofer mentioned this pull request Feb 8, 2022
Merged
@teo-tsirpanis

This comment was marked as resolved.

Sign in to view
@teo-tsirpanis teo-tsirpanis force-pushed the sdc-remove-unix branch 2 times, most recently from 3f4fe59 to 966d6bd Compare February 11, 2022 20:27
@ViktorHofer
Copy link
Member

@teo-tsirpanis #64500 which got just merged caused a conflict which I just resolved. Hope that's ok.

@teo-tsirpanis
Copy link
Contributor Author

Thanks @ViktorHofer.

@teo-tsirpanis teo-tsirpanis force-pushed the sdc-remove-unix branch 2 times, most recently from 2893c32 to 608394e Compare February 15, 2022 13:24
@teo-tsirpanis
Copy link
Contributor Author

CI is green (failures are non-Windows and unrelated). @safern is there anything else to do? This PR is getting more and more conflicts as it stays open.

@teo-tsirpanis
Copy link
Contributor Author

Conflicts are resolved.

@danmoseley
Copy link
Member

@dotnet/area-system-drawing as @safern changed teams, this will need a different one of you to sign off.

@runfoapp runfoapp bot mentioned this pull request Feb 23, 2022
Closed
@teo-tsirpanis teo-tsirpanis force-pushed the sdc-remove-unix branch 3 times, most recently from 34379c8 to 1c0ca40 Compare March 15, 2022 10:46
@teo-tsirpanis
Copy link
Contributor Author

Great, CI now passes. Can someone review?

BinaryFormatter on some enum types breaks on non-Windows because the TypeForwardedFromAttribute is missing, but I made these test cases run only on Windows. I hope there's no problem.

@danmoseley danmoseley requested a review from ericstj March 23, 2022 04:14
@danmoseley
Copy link
Member

we will need a new reviewer with Santi gone.

@teo-tsirpanis teo-tsirpanis changed the title Remove the Unix code from System.Drawing.Common. Clean-up System.Drawing.Common and remove the Unix code. Apr 19, 2022
teo-tsirpanis and others added 2 commits April 19, 2022 14:10
@teo-tsirpanis @ViktorHofer
Remove TargetsAnyOS.
df7a067 
Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@ViktorHofer
SDC project file clean-up
b61661f
ViktorHofer
ViktorHofer reviewed Apr 19, 2022
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went over all the changes. This looks REALLY good. Thanks for the great work. The remaining work now is to:

  1. Switch all the !! null checks to their previous state. For most, you could probably use ArgumentNullException.ThrowIfNull(myString);.
  2. Make sure that the remaining questions are answered.

After that we can get the PR merged :)

PS: I hope you don't mind that I pushed one commit to your branch to clean-up the SDC project file further.

@ViktorHofer ViktorHofer mentioned this pull request Apr 19, 2022
Closed
ViktorHofer
ViktorHofer approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Congratulations and thank you for your contribution 🎉🎉🎉 We very much appreciate the time and effort that you put into this change.

@ViktorHofer ViktorHofer merged commit 5b55b06 into dotnet:main Apr 20, 2022
@teo-tsirpanis teo-tsirpanis deleted the sdc-remove-unix branch April 20, 2022 08:00
@ViktorHofer
Copy link
Member

If you want to pick up another task, there are plenty ones in the backlog that are marked as up-for-grabs. And I will promise that future changes won't take us that long to react and will hopefully cause less merge conflicts (😥) on your side.

directhex pushed a commit to directhex/runtime that referenced this pull request Apr 21, 2022
@teo-tsirpanis @ViktorHofer @directhex
Clean-up System.Drawing.Common and remove the Unix code. (dotnet#64623
f73c6aa 
)

* Remove all Unix-specific files.

* Throw PNSE on non-Windows.

* Merge the Windows-specific files into their formerly cross-platform counterparts.

* Remove all mentions of Unix in the tests.

* Remove two always-on defines.

* Merge two item groups in the project file.
No reason to sort them; the list is already unsorted.
And rename a remaining formerly Windows-specific file.

* Remove the NoCOMWrappers files.

* Fail on unsupported platforms when a library is trying to be loaded.

* Fix compile errors.

* Small changes in the project file.

* Remove two meaningless asserts.

* Run BinaryFormatter tests on SDC types only on Windows.

* Use `[ThreadStatic]` in Gdip.ThreadData.

* Remove `TargetsAnyOS`.

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>

* SDC project file clean-up

* Remove `!!` from System.Drawing.Common.

Co-authored-by: Viktor Hofer <viktor.hofer@microsoft.com>
@danmoseley
Copy link
Member

I also wanted to add my belated thanks here @teo-tsirpanis. This was a lot of work.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@safern safern safern left review comments

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub left review comments

@ViktorHofer ViktorHofer ViktorHofer approved these changes

@ericstj ericstj Awaiting requested review from ericstj

@carlossanlop carlossanlop Awaiting requested review from carlossanlop

Assignees

@ViktorHofer ViktorHofer

Labels
area-System.Drawing community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@teo-tsirpanis @safern @ViktorHofer @danmoseley @carlossanlop @stephentoub